Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ChemMaster+ #2778

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

ChemMaster+ #2778

wants to merge 26 commits into from

Conversation

sleepyyapril
Copy link
Contributor

@sleepyyapril sleepyyapril commented Jan 20, 2025 •
edited
Loading

About the PR

Changes how the ChemMaster works:

  1. Removes the amount buttons and instead uses a textbox that resets whenever a value is entered or focus ends.
  2. Shrinks the ChemMaster again.
  3. Adds sorting options, including for quantity, last added, and a sort option for the (default) alphabetical order.
  4. Sorting options save via the ChemMaster itself, not per-user.
  5. Adds a pill buffer for all my pill-makers!

Why / Balance

Sorting the ChemMaster by default is bad, let people have the right to choose how it sorts.
Also lets you set your own quantity to transfer/discard.

Technical details

  1. Adds TransferringAmount and SortMethod to ChemMasterComponent, to save separately for each ChemMaster.
  2. Edits ChemMasterUiState to support the above.
  3. Shrinks the ChemMaster's UI because we no longer need that much width.

Mostly UI changes.

Media

Video showcase:
https://ptb.discord.com/channels/968983104247185448/1206353544186171482/1333324287188402277

Requirements

  • I have tested all added content and changes.
  • I have added media to this PR or it does not require an ingame showcase.

Breaking changes

Changelog

🆑

  • add: Added ChemMaster sorting and a custom amount input.
  • add: Added a ChemMaster pill buffer.

@sleepyyapril @VMSolidus
ChemMaster+ (DeltaV-Station#1585)
7b5b033 
<!--
This is a semi-strict format, you can add/remove sections as needed but
the order/format should be kept the same
Remove these comments before submitting
-->

<!--
Explain this PR in as much detail as applicable

Some example prompts to consider:
How might this affect the game? The codebase?
What might be some alternatives to this?
How/Who does this benefit/hurt [the game/codebase]?
-->

Changes how the ChemMaster works:
1. Removes the amount buttons and instead uses a textbox that resets
whenever a value is entered or focus ends.
2. Shrinks the ChemMaster again.
3. Adds sorting options, including for quantity, last added, and a sort
option for the (default) alphabetical order.
4. Sorting options save via the ChemMaster itself, not per-user.

Video showcase:
https://discord.com/channels/1218698320155906090/1218698321053356060/1330129166384894046

---

<!--
You can add an author after the `:cl:` to change the name that appears
in the changelog (ex: `:cl: Death`)
Leaving it blank will default to your GitHub display name
This includes all available types for the changelog
-->

:cl:
- add: Added sorting options to the ChemMaster.
- add: Added the ability to input custom amounts into the ChemMaster via
a textbox that resets on change.
- tweak: The width of the ChemMaster UI has been lowered.
- remove: Removed quantity buttons from the ChemMaster.

---------

Co-authored-by: VMSolidus <[email protected]>
@sleepyyapril sleepyyapril requested a review from a team as a code owner January 20, 2025 00:48
@github-actions github-actions bot added S: Needs Review size/L 256-1023 lines Changes: UI Changes: C# Changes any cs files Changes: Localization Changes any ftl files labels Jan 20, 2025
@sleepyyapril
Copy link
Contributor Author

I could theoretically upstream.

@ThataKat
Copy link
Contributor

Direction is reviewing, get back to you in about 24 hours!

@rosieposieeee
Copy link
Contributor

might be a PITA, but this exclusively touches upstream files, so i would suggest opening a parallel PR and seeing if that gets accepted

@sleepyyapril
Copy link
Contributor Author

sleepyyapril commented Jan 21, 2025 •
edited
Loading

might be a PITA, but this exclusively touches upstream files, so i would suggest opening a parallel PR and seeing if that gets accepted

Not able to until one of the ChemMaster PR snipe changes gets merged.

@sleepyyapril
Copy link
Contributor Author

sleepyyapril commented Jan 21, 2025 •
edited
Loading

I'm going over this, and I'm likely going to redo the entire ChemMaster systems as a whole. You can merge this as-is, though.

@dvir001
Copy link
Contributor

dvir001 commented Jan 21, 2025

I'm going over this, and I'm likely going to redo the entire ChemMaster systems as a whole. You can merge this as-is, though.

No comments, no merge.

@ThataKat
Copy link
Contributor

Seen closed. Direction recommends trying this type of change upstream first anyhow.

@sleepyyapril
Copy link
Contributor Author

sleepyyapril commented Jan 22, 2025 •
edited
Loading

Seen closed. Direction recommends trying this type of change upstream first anyhow.

Again, as stated, I can't. Will certainly try when that other PR is merged on WizDen's side.

@sleepyyapril sleepyyapril reopened this Jan 27, 2025
sleepyyapril and others added 5 commits January 27, 2025 00:33
@sleepyyapril
ChemMaster Improvements (DeltaV-Station#1650)
5454bf7 
Hate having to use the same buffer for pills? Want sorting and container
solutions for output? No need to wait _any_ longer! A brand new pill
buffer, a sort feature _for_ the new pill buffer **and** you have your
amount buttons back!

Showcase:
https://ptb.discord.com/channels/1218698320155906090/1218698321053356060/1332224976803074123

🆑
- add: A new ChemMaster experience has been granted to the people of
Einstein Engines. Includes a pill buffer!
@sleepyyapril
Fixed network fuckery
291867f
@sleepyyapril @VMSolidus
Fix Chemmaster Two (DeltaV-Station#1667)
1d38c0c 
simplify this shitcode

i borked it last time, sorry

---------

Signed-off-by: sleepyyapril <[email protected]>
Co-authored-by: VMSolidus <[email protected]>
@sleepyyapril
Merge branch 'master' of https://github.com/DeltaV-Station/Delta-v in…
19a842d 
…to port-chemmaster-plus
@sleepyyapril
Use separate counts for the separate buffers.
7ec1509
@sleepyyapril sleepyyapril requested a review from a team as a code owner January 27, 2025 06:32
@github-actions github-actions bot added the Changes: YML Changes any yml files label Jan 27, 2025
@sleepyyapril
Copy link
Contributor Author

Re-opened with new video showcase

@sleepyyapril
Copy link
Contributor Author

How many comments is too many?

@ThataKat
Copy link
Contributor

ThataKat commented Feb 1, 2025

Reviewing, 24-48 hours

deltanedas
deltanedas requested changes Feb 1, 2025
View reviewed changes
Content.Client/Chemistry/UI/ChemMasterBoundUserInterface.cs Outdated Show resolved Hide resolved
Content.Client/Chemistry/UI/ChemMasterWindow.xaml Outdated Show resolved Hide resolved
Content.Server/Chemistry/Components/ChemMasterComponent.cs Outdated Show resolved Hide resolved
Content.Server/Chemistry/EntitySystems/ChemMasterSystem.cs Outdated Show resolved Hide resolved
Content.Server/Chemistry/EntitySystems/ChemMasterSystem.cs Outdated Show resolved Hide resolved
Content.Shared/Chemistry/SharedChemMaster.cs Outdated Show resolved Hide resolved
Content.Shared/Chemistry/SharedChemMaster.cs Outdated Show resolved Hide resolved
Content.Shared/Chemistry/SharedChemMaster.cs Outdated Show resolved Hide resolved
Content.Shared/Chemistry/SharedChemMaster.cs Outdated Show resolved Hide resolved
Content.Shared/Chemistry/SharedChemMaster.cs Outdated Show resolved Hide resolved
deltanedas
deltanedas requested changes Feb 1, 2025
View reviewed changes
Copy link
Member

@deltanedas deltanedas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all these comments should be // DeltaV not Delta-v

Content.Client/Chemistry/UI/ChemMasterBoundUserInterface.cs Show resolved Hide resolved
deltanedas
deltanedas requested changes Feb 2, 2025
View reviewed changes
Resources/Locale/en-US/chemistry/components/chem-master-component.ftl
@@ -10,7 +10,7 @@ chem-master-bound-user-interface-title = ChemMaster 4000

chem-master-window-input-tab = Input
chem-master-window-output-tab = Output
chem-master-window-container-label = Container
chem-master-window-container-label = Container
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

untouch

Resources/Locale/en-US/_DV/chemistry/components/chem-master-component.ftl
chem-master-window-transferring-label = Transferring: [color={$color}]{$quantity}[/color]
chem-master-window-transferring-default-label = Transferring: [color=#ffffff]50[/color]
chem-master-window-reagent-move-button = Move
chem-master-window-discard-button = Discard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file needs trailing newline

Content.Shared/Chemistry/SharedChemMaster.cs
Comment on lines +153 to +163
public ChemMasterBoundUserInterfaceState( // DeltaV
ContainerInfo? inputContainerInfo,
IReadOnlyList<ReagentQuantity> bufferReagents,
IReadOnlyList<ReagentQuantity> pillBufferReagents,
FixedPoint2 bufferCurrentVolume,
FixedPoint2 pillBufferCurrentVolume,
uint selectedPillType,
uint pillDosageLimit,
bool updateLabel,
int sortMethod,
int transferringAmount)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still reformats the constructor, keep it how it was and move all deltav fields to the end

@ThataKat
Copy link
Contributor

ThataKat commented Feb 3, 2025

Working this one out with the author through discord channels, review still in progress

Lyndomen
Lyndomen approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@Lyndomen Lyndomen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction Approved, but requesting a do not merge till April gives the go ahead that all the requested features have been ported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deltanedas deltanedas deltanedas requested changes

@Lyndomen Lyndomen Lyndomen approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Changes: C# Changes any cs files Changes: Localization Changes any ftl files Changes: UI Changes: YML Changes any yml files S: Needs Review size/L 256-1023 lines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sleepyyapril @ThataKat @rosieposieeee @dvir001 @deltanedas @Lyndomen